feat: script builder#163
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces a new ChangesNumscript builder DSL
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BuildProgram
participant env
participant StmtSend
participant renderVars
Caller->>BuildProgram: BuildProgram(StmtSend(monetary, source, dest))
BuildProgram->>env: newEnv() — init pools, varsEnv, builder
BuildProgram->>StmtSend: render(env, indentLevel=0)
StmtSend->>env: WriteString "send [asset amount]"
StmtSend->>env: WriteString "(source = ..."
StmtSend->>env: render Source (SrcAccount / SrcInorder / SrcColored)
StmtSend->>env: render Destination (DestAccount)
StmtSend->>env: WriteString ")"
BuildProgram->>renderVars: renderVars(env, accountsPool, assetsPool, ...)
renderVars->>renderVars: iterate pooled IDs → emit "account $account_0 = ..."
renderVars-->>BuildProgram: vars block string
BuildProgram-->>Caller: (knownBindings, varsEnv, vars+body)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #163 +/- ##
==========================================
+ Coverage 67.81% 68.48% +0.67%
==========================================
Files 49 56 +7
Lines 5223 5417 +194
==========================================
+ Hits 3542 3710 +168
- Misses 1472 1495 +23
- Partials 209 212 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The new builder can emit invalid scripts when builder values are reused, and its typed variable API does not enforce the advertised variable type at compile time. These are correctness issues in the newly introduced public API.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
✅ Approve — automated review
No discrete correctness issues were identified in the introduced builder implementation based on the diff and repository context.
No findings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@builder/vars.go`:
- Around line 57-59: The FillAccount method and similar methods (at lines 62-64,
67-69, 72-74) silently ignore failed variable lookups by discarding the success
boolean returned from the map access operation using a blank identifier. Instead
of ignoring the second return value from v.bindings lookups, capture it and
validate that the variable binding exists, returning an error or failing fast if
the variable is not found in the bindings map. Apply this validation pattern to
all affected methods to ensure missing variable bindings are caught immediately
rather than propagating invalid empty string names downstream.
- Around line 72-74: The FillNumber function does not check if the bi parameter
is nil before calling bi.String() on line 74, which will cause a panic if bi is
nil. Add an explicit nil check for the bi parameter before calling bi.String(),
and return an appropriate string representation (such as empty string or "nil")
when bi is nil to prevent the panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd1c5ee4-c3ef-4a1e-8998-0fd9a9e65bf0
📒 Files selected for processing (4)
builder/builder.gobuilder/builder_test.gobuilder/expression.gobuilder/vars.go
🚧 Files skipped from review as they are similar to previous changes (2)
- builder/expression.go
- builder/builder_test.go
| func (v VarsEnv) FillAccount(var_ *Var[ExprTypeAccount], account string) (string, string) { | ||
| name, _ := v.bindings[anyVar(var_)] | ||
| return name, account |
There was a problem hiding this comment.
Fail fast when a variable is not bound in VarsEnv.
At Line 58/63/68/73, missing keys are silently converted into "" names. That can emit invalid bindings downstream without any explicit failure signal. Validate lookup success and fail fast.
Suggested fix
func (v VarsEnv) FillAccount(var_ *Var[ExprTypeAccount], account string) (string, string) {
- name, _ := v.bindings[anyVar(var_)]
+ name, ok := v.bindings[anyVar(var_)]
+ if !ok {
+ panic("FillAccount called with variable not allocated in program")
+ }
return name, account
}
func (v VarsEnv) FillAsset(var_ *Var[ExprTypeAsset], asset string) (string, string) {
- name, _ := v.bindings[anyVar(var_)]
+ name, ok := v.bindings[anyVar(var_)]
+ if !ok {
+ panic("FillAsset called with variable not allocated in program")
+ }
return name, asset
}
func (v VarsEnv) FillString(var_ *Var[ExprTypeString], str string) (string, string) {
- name, _ := v.bindings[anyVar(var_)]
+ name, ok := v.bindings[anyVar(var_)]
+ if !ok {
+ panic("FillString called with variable not allocated in program")
+ }
return name, str
}
func (v VarsEnv) FillNumber(var_ *Var[ExprTypeNumber], bi *big.Int) (string, string) {
- name, _ := v.bindings[anyVar(var_)]
+ name, ok := v.bindings[anyVar(var_)]
+ if !ok {
+ panic("FillNumber called with variable not allocated in program")
+ }
return name, bi.String()
}Also applies to: 62-64, 67-69, 72-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@builder/vars.go` around lines 57 - 59, The FillAccount method and similar
methods (at lines 62-64, 67-69, 72-74) silently ignore failed variable lookups
by discarding the success boolean returned from the map access operation using a
blank identifier. Instead of ignoring the second return value from v.bindings
lookups, capture it and validate that the variable binding exists, returning an
error or failing fast if the variable is not found in the bindings map. Apply
this validation pattern to all affected methods to ensure missing variable
bindings are caught immediately rather than propagating invalid empty string
names downstream.
| func (v VarsEnv) FillNumber(var_ *Var[ExprTypeNumber], bi *big.Int) (string, string) { | ||
| name, _ := v.bindings[anyVar(var_)] | ||
| return name, bi.String() |
There was a problem hiding this comment.
Guard FillNumber against nil *big.Int.
At Line 74, bi.String() will panic when bi == nil. Add an explicit nil check before conversion.
Suggested fix
func (v VarsEnv) FillNumber(var_ *Var[ExprTypeNumber], bi *big.Int) (string, string) {
name, _ := v.bindings[anyVar(var_)]
+ if bi == nil {
+ panic("FillNumber called with nil *big.Int")
+ }
return name, bi.String()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (v VarsEnv) FillNumber(var_ *Var[ExprTypeNumber], bi *big.Int) (string, string) { | |
| name, _ := v.bindings[anyVar(var_)] | |
| return name, bi.String() | |
| func (v VarsEnv) FillNumber(var_ *Var[ExprTypeNumber], bi *big.Int) (string, string) { | |
| name, _ := v.bindings[anyVar(var_)] | |
| if bi == nil { | |
| panic("FillNumber called with nil *big.Int") | |
| } | |
| return name, bi.String() | |
| } |
🧰 Tools
🪛 GitHub Actions: Default / Dirty
[error] 55-74: CI failed because the repository had uncommitted changes. The step 'git status --porcelain' detected modifications and exited with code 1 after showing 'There are changes in the repository' and running 'git diff' for modified file 'builder/vars.go'. Diff indicates changes to variable binding assignments in FillAccount/FillAsset/FillString/FillNumber.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@builder/vars.go` around lines 72 - 74, The FillNumber function does not check
if the bi parameter is nil before calling bi.String() on line 74, which will
cause a panic if bi is nil. Add an explicit nil check for the bi parameter
before calling bi.String(), and return an appropriate string representation
(such as empty string or "nil") when bi is nil to prevent the panic.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
✅ Approve — automated review
No discrete correctness issues were identified in the added builder package based on the diff and surrounding parser/runtime expectations.
No findings.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
✅ Approve — automated review
No discrete correctness issues were found in the added builder implementation based on the diff and existing Numscript grammar/API expectations.
No findings.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The new builder can generate invalid Numscript for a typed API call by applying the unbounded overdraft clause to source forms where the grammar does not allow it.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
✅ Approve — automated review
I did not identify any discrete correctness issues in the added builder API from the diff. The generated scripts and variable binding approach appear consistent with the existing Numscript grammar and runtime expectations.
No findings.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The builder can generate colored-source scripts that are rejected by the default interpreter execution path because the required feature flag is not included or surfaced.
| ) Source { | ||
| return func(env *env, w int) { | ||
| accountExpr(env, w) | ||
| env.builder.WriteString(" \\ ") |
There was a problem hiding this comment.
🟠 [major] Enable asset-colors when rendering colored sources
When callers use SrcColored/SrcColoredOverdraft and execute the returned script via the normal Parse(...).Run path, the interpreter rejects it because colored sources require the experimental-asset-colors feature flag, but BuildProgram never emits a #![feature(...)] declaration or returns required flags. The builder should track this feature or otherwise expose a way to make generated colored scripts runnable.
4 participants
No description provided.